Added option to specify cardiac period #127
Conversation
|
@rjrios915 @JonathanPham Please make sure the tests are passing! Also, just wondering if it might be better to specify the cardiac cycle period within the relevant block that is driving the cardiac cycle (like the cardiac chamber)? For example, the currently implemented |
… into RegChamber
|
@rjrios915 @JonathanPham Not sure why the tests are still failing but can you rebase this branch with the master so it it using the most recent version please? |
|
Hello! All test cases except the data visualization should now be passing. As for where the cardiac period is defined, I think it could be useful to do so globally. The solver already utilizes a global period to calculate step size, number of cycles, and the time per cycle when outputting a single cardiac cycle even when there is no heart block, so I was thinking adding it to simulation parameters would make it easy for users to change the value if needed. |
There was a problem hiding this comment.
@rjrios915 We should make sure your changes are consistent with what the code is currently doing. I have commented on specific changes and added links to where they might be inconsistent with the current code. Here's a quick summary:
Currently, model->cardiac_cycle_period is initialized as -1.0 here:
svZeroDSolver/src/model/Model.h
Line 94 in edd6156
model->cardiac_cycle_period remains -1.0 until it is specified by a block, at which point we make sure that the newly specified value matches the value read previously from any other block (if any). I have provided examples of how that is checked in my comments below.
If no value has been specified after all blocks have been read, a default value of 1.0 is set. This is in Model::finalize and I have also provided a link to that code block in my comments below.
Please make sure your current changes are consistent with that. Mainly, the default should be 1.0 and you should check that any newly specified value is consistent with previously specified values (if any).
|
Hello! We ran into the issue where the initialization of cardiac period to 1 within the model and simulation parameters separately cause an error when one would automatically be 1 but the other some valid non 1 value. To resolve this, both the model and parameters initialize the cardiac period to -1 unless specified elsewhere. If both values are -1, then solver.h sets the value to 1 (like what was happening in Model::Finalize). If both values are larger than 0, there is a check to make sure the values are consistent. If one is -1 and the other is greater than 0, then that value is set as the cardiac cycle. The other idea we had was to pass the parameters to the model constructor so this process could happen like originally when the model is initialized, but I was unsure of how to go about this. Let me know your thoughts! All the tests should be passing. |
|
@rjrios915 ensure that documentation for cardiac period has been added |
|
What else needs to be done to merge this (if we still want to merge it)? |
|
We do still want to merge this. I will meet with @rjrios915 this week to finalize. Ricky - are all the tests passing and do we still need to address karthik's earlier comments? |
|
You can format your code automatically, e.g., in VS Code: https://code.visualstudio.com/docs/cpp/cpp-ide |
|
all tests are passing here - @menon-karthik are we good to merge this? There was a new test case added to make sure we are catching mismatched cardiac period errors, maybe you want to look over that quickly. |
|
still need 1 more review on this to merge it I think |
|
@rjrios915 It appears that this branch got mixed up with the branch in #168 |
ncdorn
dismissed
menon-karthik’s stale review
This is now an outdated review, and the changes were addressed.
|
@mrp089 do you want to review this before we merge? |
mrp089
left a comment
mrp089
left a comment
There was a problem hiding this comment.
Minor: exclude your changes in the .github folder from this PR.
Apart from that, it looks good to me. It would be good to document the new option. Unfortunately, I have no idea where to put that after the documentation was moved away from this repository. I'll let @ktbolt sort that out.
|
@mrp089 The important thing is that users can now find the documentation ! I think most SimVascular developers have updated the web documentation, very easy to do in fact. |
6 participants
Addresses #126
Added option to specify cardiac period under simulation parameters
Current situation
Link any open issues or PRs related to this PR. Please ensure that all non-trivial PRs are first tracked and discussed in an existing GitHub issue or discussion.
Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Documentation
Please ensure that you properly document any additions.
Testing
Please ensure that the PR meets the testing requirements set by GitHub Actions.
In addition, please ensure all modified and new code is covered by tests.
Code of Conduct & Contributing Guidelines